-
Notifications
You must be signed in to change notification settings - Fork 550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cephfs: set Pool parameter to empty for Snapshot-backed volumes #4047
Conversation
// Pool parameters is optional and only used to set 'pool_layout' arguement for | ||
// subvolume and subvolume clone create commands. | ||
// Setting this to emtpy since it is not used with Snapshot-backed volume. | ||
vo.Pool = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an E2E testing to ensure it works and we will not break it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
4ec5e0a
to
c516125
Compare
/test ci/centos/mini-e2e/k8s-1.27 |
// Pool parameters is optional and only used to set 'pool_layout' argument for | ||
// subvolume and subvolume clone create commands. | ||
// Setting this to empty since it is not used with Snapshot-backed volume. | ||
vo.Pool = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this not be the same as the pool of the original volume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this not be the same as the pool of the original volume?
The original volume's pool is set only for create pvc/clone cmd as pool_layout
(pool is retrieved directly from pv attributes) and is not saved anywhere and not used anywhere else.
The parentBackingSnapVolOpts
here does not have its pool parameter set.
if vo.Pool != "" { | ||
return errors.New("cannot set pool for snapshot-backed volume") | ||
} | ||
// Pool parameters is optional and only used to set 'pool_layout' argument for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pool parameters
to Pool parameter
c516125
to
18e85a0
Compare
@Mergifyio rebase |
Set VolumeOptions.Pool parameter to empty for Snapshot-backed volumes. This Pool parameter is optional and only used as 'pool-layout' parameter during subvolume and subvolume clone create request in cephcsi and not used for Snapshot-backed volume at all. It is not saved anywhere for use in subsequent operations after create too. Therefore, We can set it to empty and not error out. Signed-off-by: rakshith-r <[email protected]>
✅ Branch has been successfully rebased |
18e85a0
to
9692995
Compare
/test ci/centos/k8s-e2e-external-storage/1.25 |
/test ci/centos/k8s-e2e-external-storage/1.26 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.25 |
/test ci/centos/mini-e2e-helm/k8s-1.26 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.25 |
/test ci/centos/mini-e2e/k8s-1.26 |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
Describe what this PR does
Set VolumeOptions.Pool parameter to empty for Snapshot-backed volumes. This Pool parameter is optional and only used as 'pool_layout' parameter during subvolume and subvolume clone create request in cephcsi and not used for Snapshot-backed volume at all.
It is not saved anywhere for use in subsequent operations after create too. Therefore, We can set it to empty and not error out.
Refer: https://docs.ceph.com/en/latest/cephfs/fs-volumes/?highlight=pool_layout#fs-subvolumes
Resolves: #3820